Already performed inside AfterEach()

After each function is doing cleanup for a global namespace which is created for all the tests, In this test I am creating a new namespace and doing cleanup at the end of the test function to ensure there is no resources are available. I tried with the Global namespace and was facing some issue so trying to create inside the test script only and clean once execution is done.

If you need a separate namespace for each test case, that should be done in BeforeEach/AfterEach. If something fails before this code, it will never be cleaned up.

Example:

ginkgo.BeforeEach(func() {
		kubeClient = k8s.KubeCtl{}
		gomega.Expect(kubeClient.SetClient()).To(gomega.BeNil())
		ns = "ns-" + common.RandSeq(10)
		ginkgo.By(fmt.Sprintf("Creating namespace: %s for admission controller tests", ns))
		var ns1, err1 = kubeClient.CreateNamespace(ns, nil)
		gomega.Ω(err1).NotTo(gomega.HaveOccurred())
		gomega.Ω(ns1.Status.Phase).To(gomega.Equal(v1.NamespaceActive))
	})

...

	ginkgo.AfterEach(func() {
		ginkgo.By("Tear down namespace: " + ns)
		err := kubeClient.TearDownNamespace(ns)
		gomega.Ω(err).NotTo(gomega.HaveOccurred())
		// call the healthCheck api to check scheduler health
		ginkgo.By("Check YuniKorn's health")
		checks, err2 := yunikorn.GetFailedHealthChecks()
		gomega.Ω(err2).ShouldNot(gomega.HaveOccurred())
		gomega.Ω(checks).Should(gomega.Equal(""), checks)
	})

Why do we start with a 20 second sleep? Looks very arbitrary.

The Cleanup for other tests is taking time and because of that the test which I added is failing intermittently for a few k8s versions. Added a sleep before the execution of test so it will trigger once the cleanup is done and resources are available to allocate.

That's interesting. I think the reason is that tests are not using different namespaces. See my comment below, you need to create/destroy a namespace for each test. This is what solved many problems for the admission controller tests, they often interfered with each other, debugging was difficult. A new namespace for every test solved it.

Hi Peter, Checked your comments and tried that approach but that required a lot of changes since we are using common namespace for all the tests so need to update each namespace as well. Also facing some issue with your approach.

Why does it need a lot of changes? That's what we're doing everywhere. Tests should be isolated by different namespaces. This 20 seconds sleep is only acceptable as a workaround. I'll try to see how difficult that is, but at least this must be addressed in a follow-up JIRA. This cannot stay as is.

OK, I can see what the problem is. I'll create a JIRA to separate the test cases. It's unacceptable to have all tests in a single namespace. They can easily interfere with each other.

https://issues.apache.org/jira/browse/YUNIKORN-2247

"WithRequiredNode" or "WithNodeSelector" sounds better

Acknowledged.

This part is very suspicious. You're using the "RequiredNode" setting here, which will trigger the "simple" (aka. RequiredNode) preemption logic inside Yunikorn. That is not the generic preemption logic that Craig worked on.

Please check if the console log of Yunikorn contains this:

log.Log(log.SchedApplication).Info("Triggering preemption process for daemon set ask",
		zap.String("ds allocation key", ask.GetAllocationKey()))

If this is the case (which is VERY likely), changes are necessary.

A trivial solution is to enhance the RequiredNode field, it has to be a struct like:

type RequiredNode struct {
  Node      string
  DaemonSet bool
}

type SleepPodConfig struct {
...
	Mem          int64
	RequiredNode RequiredNode 
	Optedout     AllowPreemptOpted
}

If DaemonSet == false, then this code doesn't run:

So this line effectively becomes:

sandbox1Configs = append(sandbox1Configs,
 k8s.SleepPodConfig{
    Name: fmt.Sprintf("sleepjob%d", i+1),
    NS: devNew, Mem: sleepPodMemLimit2,
    Time: time,
    Optedout: k8s.Allow,
    Labels: map[string]string{"queue": "root.sandbox1"},
    RequiredNode: RequiredNode{Node: nodeName, DeamonSet: false}
    }
)

The existing tests inside simple_preemptor_test.go are affected.

Have you looked at this? Again, the tests should address YUNIKORN-2068. RequiredNodePreemptor does not use the predicates.